Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ Make "go to source definition" work #658

Merged
merged 15 commits into from
May 7, 2024
Merged

♻️ Make "go to source definition" work #658

merged 15 commits into from
May 7, 2024

Conversation

coyotte508
Copy link
Member

@coyotte508 coyotte508 commented May 6, 2024

Sorry for all the codeowners reviews

Make "go to source definition" work by using the package, thanks to .d.ts.map files generated by tsc.

Defer type generation to tsc instead of tsup, since tsup is not able to generate .d.ts.map. Also removed shared package - since we don't want to publish it, having "go to source" with it would have been problematic. Instead, "shared" files are duplicated between @huggingface/inference and @huggingface/hub.

Maybe later we can create @huggingface/webblob and @huggingface/fileblob if we want to reuse them.

See also: https://www.npmjs.com/package/dts-buddy used by svelte-kit

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are all those files intended?

Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm in principle

@coyotte508 coyotte508 force-pushed the go-to-source branch 2 times, most recently from c4c3c86 to 8c272d1 Compare May 6, 2024 16:19
@coyotte508 coyotte508 marked this pull request as ready for review May 6, 2024 17:42
@@ -25,6 +25,7 @@ export default defineConfig(({ mode }) => {
emitCss: false,
compilerOptions: {
hydratable: true,
// @ts-expect-error to check cc @krampstudio
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @krampstudio , apparently the correct type is Omit<ViteOptions, "generate">, for now bypassed the error (not sure the error is relevant or not)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed the type in vite-plugin-svelte omits it, and the doc says it's non-configurable.
But we use it in lib mode and the type might not be accurate since the options are passed down to the svelte compiler

"prepublishOnly": "pnpm run build",
"build": "tsup src/index.ts --format cjs,esm --clean --dts && pnpm run inference-codegen",
"prepublishOnly": "pnpm run inference-codegen && git diff --name-only --exit-code src && pnpm run build",
"build": "tsup src/index.ts --format cjs,esm --clean && tsc --emitDeclarationOnly --declaration",
Copy link
Member Author

@coyotte508 coyotte508 May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Wauplin @SBrandeis I changed the codegen to automatically run in @huggingface/tasks only during prepublish (publish will error if not up to date)

We would do the same for tgi codegen

It avoids doing it during every pnpm install but should be done manually before publishing (or maybe we could add a lint check that it's up to date in the Lint CI)

@coyotte508
Copy link
Member Author

Going ahead and merging - let me know if any issue arises from it

@coyotte508 coyotte508 merged commit 99bbf1f into main May 7, 2024
5 checks passed
@coyotte508 coyotte508 deleted the go-to-source branch May 7, 2024 12:37
@julien-c
Copy link
Member

julien-c commented May 7, 2024

is the Adding a package section in CONTRIBUTING.md still up to date with this change?

@coyotte508
Copy link
Member Author

is the Adding a package section in CONTRIBUTING.md still up to date with this change?

good point, updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants